Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove unused code #2609

Merged
merged 4 commits into from
Aug 17, 2017
Merged

remove unused code #2609

merged 4 commits into from
Aug 17, 2017

Conversation

forki
Copy link
Member

@forki forki commented Aug 12, 2017

No description provided.

@forki forki requested a review from matthid August 12, 2017 08:33
@matthid
Copy link
Member

matthid commented Aug 12, 2017

Yes the unused code was a oversight temporarily used for debugging, nice catch.

Did you profile/measure the switch to hashset? I'm asking because the list is always quite small, so the hashset might not even be optimal. At least this code did not yield any alarm bells in the profiler at the time...

@matthid
Copy link
Member

matthid commented Aug 12, 2017 

1) Failed : Paket.LockFile.ParserSpecs.should parse lock file many frameworks
  Expected: "NUGET
  remote: https://www.nuget.org/api/v2
    CommonServiceLocator (1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+monoandroid+xamarinios+win8+wp8+wpa81)
    MvvmLightLibs (5.2)
      CommonServiceLocator (>= 1.0) - restriction: || (== net35) (== sl4)
      CommonServiceLocator (>= 1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+monoandroid+xamarinios+win8+wp8+wpa81)"
Actual: "NUGET
  remote: https://www.nuget.org/api/v2
    CommonServiceLocator (1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+win8+wp8+wpa81)
    MvvmLightLibs (5.2)
      CommonServiceLocator (>= 1.0) - restriction: || (== net35) (== sl4)
      CommonServiceLocator (>= 1.3) - restriction: || (== sl5) (>= net40) (>= portable-net45+win8+wp8+wpa81)"
  Expected string length 406 but was 360. Strings differ at index 135.
  Expected: "...(>= net40) (>= portable-net45+monoandroid+xamarinios+win8+..."
  But was:  "...(>= net40) (>= portable-net45+win8+wp8+wpa81)\r\n    MvvmLig..."

This is actually correct as you can see in http://nugettoolsdev.azurewebsites.net/4.0.0/parse-framework?framework=portable-net45%2Bmonoandroid%2Bxamarinios%2Bwin8%2Bwp8%2Bwpa81

Not exactly sure how that PR fixes the portable detection.

Same for

3) Failed : Paket.LockFile.ParserSpecs.should parse portable lockfile
  Expected: ">= portable-net40+win8+wp8+sl5"
Actual: ">= portable-net40+sl5+win8+wp8"
  String lengths are both 30. Strings differ at index 18.
  Expected: ">= portable-net40+win8+wp8+sl5"
  But was:  ">= portable-net40+sl5+win8+wp8"
  -----------------------------^
at FsUnit.Extensions.shouldEqual[a](a expected, a actual) in C:\projects\paket\paket-files\test\forki\FsUnit\FsUnit.fs:line 10

I'm not exactly sure about


2) Failed : Paket.LockFile.ParserSpecs.should parse new restrictions || (&& (< net45) (>= portable-net45+win8+wp8+wpa81)) (&& (< portable-net45+monoandroid+monotouch+xamarinios+xamarinmac+win8+wp8+wpa81) (>= portable-net45+win8+wp8+wpa81))
...
  Expected string length 1344 but was 1023. Strings differ at index 308.
  Expected: "...t.Bcl (1.1.10) - restriction: || (&& (< net45) (>= portabl..."
  But was:  "...t.Bcl (1.1.10) - restriction: && (< net45) (>= portable-ne..."
  --------------------------------------------^
at FsUnit.Extensions.shouldEqual[a](a expected, a actual) in C:\projects\paket\paket-files\test\forki\FsUnit\FsUnit.fs:line 10

But maybe paket now detects that one of the platforms is a proper subset of the other... On the other hand it might be a incorrect detection of portable-net45+monoandroid+monotouch+xamarinios+xamarinmac+win8+wp8+wpa81 to one of the default profiles. As NuGet cannot assign a default portable profile (http://nugettoolsdev.azurewebsites.net/4.0.0/parse-framework?framework=portable-net45%2Bmonoandroid%2Bmonotouch%2Bxamarinios%2Bxamarinmac%2Bwin8%2Bwp8%2Bwpa81) We should not do it as well.

@matthid
Copy link
Member

matthid commented Aug 12, 2017

Let me know if that makes any sense at all :)

@forki
Copy link
Member Author

forki commented Aug 12, 2017 via email

@matthid
Copy link
Member

matthid commented Aug 12, 2017

Ah now I see.

This is really weird, maybe a nuget bug.
"portable-net45+monoandroid+monotouch+xamarinios+xamarinmac+win8+wp8+wpa81" -> No detection

But if the non pcl stuff is removed:

"portable-net45+win8+wp8+wpa81" -> Profile 259

I think we would need to ask a NuGet expert about this.
If we want to mirror NuGet it will be tough without understanding the underlying logic...

@matthid
Copy link
Member

matthid commented Aug 12, 2017

Ok latest NuGet detects 259 as well: http://nugettoolsdev.azurewebsites.net/4.3.0-preview4/parse-framework?framework=portable-net45%2Bmonoandroid%2Bmonotouch%2Bxamarinios%2Bxamarinmac%2Bwin8%2Bwp8%2Bwpa81

So this is probably fine.

matthid
matthid approved these changes Aug 12, 2017
View reviewed changes
Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once it's green obviously ;)

@forki
Copy link
Member Author

forki commented Aug 12, 2017 via email

@matthid
Copy link
Member

matthid commented Aug 12, 2017 •
edited
Loading

What do you mean? Updating the tests?

@matthid
Copy link
Member

matthid commented Aug 12, 2017

Maybe remove the last commit

@forki
Copy link
Member Author

forki commented Aug 12, 2017 via email

@matthid
Copy link
Member

matthid commented Aug 12, 2017

Yes strictly speaking they are probably a bugfix for some non-existing pcl users ;)

@forki
Revert "Use sets"
7069575 
This reverts commit 49164d4.
@forki
Copy link
Member Author

forki commented Aug 12, 2017

Ok. Reverted sets

@forki forki merged commit 7069575 into master Aug 17, 2017
@matthid matthid deleted the pcl branch July 2, 2019 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid matthid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@forki @matthid